Conversation
jmestwa-coder
force-pushed
the
sql-acl-handshake-oob-read
branch
2 times, most recently
from
9fbba8d to
79b17c9
Compare
gkodinov
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution. This is a preliminary review.
Would you consider describing the problem and a sequence of actions that lead to it?
Preferably there should be a regression test for it.
sql/sql_acl.cc
Outdated
| } | ||
| else if (!(thd->client_capabilities & CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA)) | ||
| { | ||
| if (passwd >= (char*) net->read_pos + pkt_len) |
There was a problem hiding this comment.
This check, if it needs to be done, needs to be done after assigning passwd. Or, maybe, instead of doing this check, how about constraining strend to never go out of buffer? Well, IMHO you do not even need to do that: higher level code ensures that all packages are actually null-terminated iirc.
Which brings me to my next question: do you have a test to demonstrate the problem?
If you can't hit this by fuzzing the network data (as I suppose you wouldn't be able to), then convert this into an assert to make sure the packet is properly terminated.
There was a problem hiding this comment.
You’re right that the packet buffer is guaranteed to be null-terminated by higher-level code so it’s hard to demonstrate this as a real runtime issue.
I’ve updated the change to use a DBUG_ASSERT instead, to document and enforce the assumption that passwd stays within the packet bounds.
jmestwa-coder
force-pushed
the
sql-acl-handshake-oob-read
branch
2 times, most recently
from
e878335 to
b2cbd8c
Compare
Replace runtime check with DBUG_ASSERT to document the invariant that passwd remains within the packet buffer, as guaranteed by higher-level code.
jmestwa-coder
force-pushed
the
sql-acl-handshake-oob-read
branch
from
b2cbd8c to
f15f504
Compare
2 participants
Summary
Fix an out-of-bounds read in
parse_client_handshake_packetwhen handling truncated authentication packets.Problem
If the username terminator is the last byte in the packet,
passwdcan reach the end of the packet buffer.The code then dereferences
*passwdbefore checking bounds, leading to a potential out-of-bounds read.Fix
Add a boundary check before the dereference:
packet_errorifpasswdis at or beyond the packet buffer endImpact
Testing
Existing tests cover malformed authentication flows but not this exact boundary case.
Happy to add a regression test if there’s a recommended way to express it.